Skip to content

Fix #6606: Do not compute S on negative Ints #7544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 15, 2019
Merged

Fix #6606: Do not compute S on negative Ints #7544

merged 1 commit into from
Nov 15, 2019

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki self-assigned this Nov 12, 2019
@nicolasstucki nicolasstucki marked this pull request as ready for review November 12, 2019 20:14
liufengyun
liufengyun reviewed Nov 14, 2019
View reviewed changes
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel this is somewhat inconsistent, as S is defined for Int, not just positive integers.

I'd like to know if there is a technical reason to restrict it to just positive numbers?

@nicolasstucki
Copy link
Contributor Author

S is the Peano successor. By definition they only work on positive numbers.

I will add some documentation.

@nicolasstucki
Copy link
Contributor Author

The only documentation we had so far was
https://github.com/lampepfl/dotty/pull/7544/files#diff-1b5c8ce1eaca91d793eef217487d5a59R1018-R1021. Which clearly states that they are natural number. Maybe it is worth considering having an actual alias for Nat with would be an intrinsinc definition for type Nat = 0 | 1 | 2 | ... | 2147483647.

liufengyun
liufengyun approved these changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, LGTM

As discussed with @nicolasstucki , it's good to document and make it coherent first. The use case for negative integers is unclear for the moment, and we can always easily change without breaking anything when justified use cases emerge.

tests/neg/i6606.scala
@@ -0,0 +1,8 @@
val t0: Int = 0
val t1: scala.compiletime.S[-1] = 0 // error
val t2: scala.compiletime.S[Int] = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me why this line type checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is because type 1 =:= S[0] <:< S[Int]

Copy link
Contributor

@liufengyun liufengyun Nov 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to figure out the precise spec for S, whether it's co-variant, contra-variant, non-variant in another issue.

@liufengyun liufengyun merged commit 8626272 into scala:master Nov 15, 2019
@liufengyun liufengyun deleted the fix-#6606 branch November 15, 2019 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liufengyun liufengyun liufengyun approved these changes

Assignees

@liufengyun liufengyun

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nicolasstucki @liufengyun